-
Notifications
You must be signed in to change notification settings - Fork 99
Support weights_disallowed field in P4Info
#563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Please provide a summary description, thanks. |
weights_determined_by_switch field in P4Info
|
Other design considerations that we had include:
Other questions
EDIT: changed the link to reference P4C version |
|
@matthewtlam you shared an internal Google link You could introduce a standard P4Runtime annotation to control whether that boolean should be set in the P4Info (similar to |
|
Thanks @antoninbas! That would be our suggestion if we want to do this boolean approach. The main question, I think, would be if there is something more expressive we might want to put here instead, though I can't immediately think of other ways in which the hash algorithm might affect what the controller programs... I suppose a 'per-group' hash algorithm (including "You choose weights") would be a way to go (as we sort of discussed on Angela's PR), but I don't actually know of any switch with close to that expressive power ;). |
|
"switch using its own hash algorithm to determine the weights" |
weights_determined_by_switch field in P4Infoweights_disallowed field in P4Info
|
@chrispsommers, @jafingerhut I've updated the PR to reflect what we discussed last meeting regarding dynamic load balancing and also updated the variable name to reflect this. cc: @jonathan-dilorenzo, @smolkaj for viz |
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
Signed-off-by: Matthew Lam <[email protected]>
|
@chrispsommers and @jafingerhut I've updated the PR with some of the comments that were discussed during today's P4 WG meeting. Please take a look |
chrispsommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@matthewtlam I just merged #560 so not sure if you need to rebase or not. I'm OK with this, but would like @jafingerhut to have a chance to do final review. Thanks for the contribution. |
jafingerhut
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the one comment I made about a typo, this looks good to me.
Signed-off-by: Matthew Lam <[email protected]>
|
@chrispsommers @jafingerhut I fixed the typo, it should be ready for merge! |
smolkaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this has LGTMs all around, going to merge.
The P4 program generally specifies a hash algorithm for use with action selectors. Previously, these did not show up in the P4Info since the choice of algorithm did not affect the control plane. However, we are now running into switches that support a new kind of algorithm called Dynamic Load Balancing (DLB) or Adaptive Routing (AR). See e.g. https://sonic.software/sai/group__SAIARS.html for some of the SAI APIs.
These algorithms have the interesting property that, instead of the controller defining the relative weights in the group, they are dynamically changing based on various measures of port quality. That means that this kind of algorithm does affect the control plane, since weights no longer make sense.
We wish to introduce this new field ,
weights_disallowed, to the P4Info to denote to the control plane that programming will have no effects on the weights (and thus, to avoid confusion, should be rejected).cc: @jonathan-dilorenzo, @smolkaj for vis